erts: improve ordered_set AVL and CA tree memory layout - #10880
erts: improve ordered_set AVL and CA tree memory layout#10880NelsonVides wants to merge 4 commits into
Conversation
CT Test Results 3 files 136 suites 50m 56s ⏱️ Results for commit bd03ca6. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
ab8d64c to
f04e46b
Compare
sverker
left a comment
sverker
left a comment
There was a problem hiding this comment.
This looks interesting.
However, I will not merge to master now for OTP 29.0 as we are quite late in the release schedule for such a large internal reworking.
Poke us if the PR hasn't been labeled with "testing" when summer comes after OTP 29.0 been released.
| #define TREE_GET_BALANCE(p) \ | ||
| ((int)((UWord)(p)->left & TREE_TAG_MASK) | \ | ||
| -((int)(((UWord)(p)->left & 2) >> 1) << 1)) |
There was a problem hiding this comment.
Could be simplified to something like this, I think
(((int)left + 1) & 3) - 1
|
@sverker hey there 👋🏽 As mentioned, pinging here, maybe we can roll this one into testing? 🙂 |
sverker
added
enhancement
testing
|
The commit with AVL balance in pointer is making the code quite messy. We want the other two commits only in this PR. |
Encode the AVL balance factor in the two low bits of the TreeDbTerm left pointer instead of a separate field, reducing memory per node on 64-bit (one word saved per node). Requires 8-byte-aligned nodes (ERTS_ALLOC_ALIGN_BYTES >= 8). The linux kernel implements a similar trick for its Red-Black trees, storing the bits for the colour at the lowest bits of the pointer to the parent.
…pointer" This reverts commit 93c6a44.
The CA tree (contention-adapting tree) backs ETS ordered_set tables.
Profiling the struct layout on 64-byte cache lines reveals two
performance problems that this commit fixes by reordering struct fields
— no code changes, no memory overhead.
Problem 1: Base node false sharing
In the old layout, lock_statistics (written by contending threads
WITHOUT the lock held) sits on the same cache line (CL1) as lock tail
fields and is_valid (read/written by the lock holder). When a contender
bumps lock_statistics, the hardware invalidates the entire 64-byte cache
line on the lock holder's core, forcing a re-fetch of unrelated lock
internals.
Fix: Move root and is_valid up (CL1, co-located with lock tail — all
accessed under the lock, no conflict). Move lock_statistics down to CL2,
isolated with only the cold free_item field. Contenders now dirty a
cache line the lock holder never touches.
Before CL1: [lock.q | lock.type | lock.rq_end | lock.tdata | lock_statistics | is_valid]
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^
lock holder reads/writes these contenders write this
→ FALSE SHARING
After CL1: [lock.q | lock.type | lock.rq_end | lock.tdata | root | is_valid]
all accessed under the lock — no cross-core conflict
After CL2: [lock_statistics | free_item]
contenders write here — isolated from lock holder
Problem 2: Route node poor cache locality
During lock-free traversal (find_base_node), each route node requires
reading: is_base_node (CL0), key.term (CL2), then left or right (CL1) —
3 cache-line fetches per node. For a tree of depth D, that is 3D
cache-line loads to reach a base node.
Fix: Move left and right to the front of the route node struct, placing
them on CL0 alongside is_base_node. The traversal loop now reads
is_base_node (CL0), key.term (CL2), then left/right (CL0, already
cached) — 2 cache-line fetches per node, a 33% reduction in cache misses
during traversal.
Before: is_base_node[CL0] → key.term[CL2] → left/right[CL1] = 3 CLs
After: is_base_node[CL0] → key.term[CL2] → left/right[CL0] = 2 CLs
Both changes are pure field reordering in erl_db_catree.h. All field
accesses in erl_db_catree.c use named fields (->u.base.root,
->u.route.left, etc.), and allocation size macros use offsetof(), so
they auto-adjust. No .c file changes required.
Lift is_valid into the DbTableCATreeNode wrapper (shared by both variants), saving 8 bytes per route node and moving key.term from cache line 2 to cache line 1. Align lock_statistics to the cache line (ERTS_CACHE_LINE_SIZE) so it sits on its own line and avoids false sharing with the lock and root when contending threads update it without holding the lock.
f04e46b to
bd03ca6
Compare
|
Hello there! I've rebased on top of the latest master, and undid that commit by git-revert, so that the original intent is not lost, in case we want to reconsider it later: I found it a very good memory saving one in case it's desired. Let me know how your tests run, if you found any performance regression or improvements :) |
4 participants
This change is divided in three commits with big descriptions, where in general the goal is to improve CPU cache lines by shuffling and padding how some structs for ordered sets are laid out. See commit messages for details.
Some benchmark results using infrastructure from ets_SUITE on an AMD Ryzen 9 9950X3D (16cores/32threads)
master
this branch
Results are ranging from 0-3% improvements depending on the scenario, at least for how biased the benchmark might actually be. I've ran these benchmarks a dozen times on both master and this branch and have gotten widely varying data for all cases, so not sure how to benchmark into statistically sound results, but at least theoretically the changes are sound (or at least I had a great experience researching them :D)